The attached testcase causes a panic in m-c 20180606-04cc917f68c5. ... #5: std::panicking::begin_panic at src/libstd/panicking.rs:538 #6: style::properties::animated_properties::_<impl style..values..animated..Animate for style..values..generics..transform..TransformOperation<style..values..computed..angle..Angle, f32, style..values..computed..length..CSSPixelLength, i32, style..values..computed..length..LengthOrPercentage>>::animate at out/properties.rs:0 #7: _<&'a mut I as core..iter..iterator..Iterator>::next at out/properties.rs:89263 #8: _<alloc..vec..Vec<T> as alloc..vec..SpecExtend<T, I>>::from_iter at src/liballoc/vec.rs:1801 #9: _<style..properties..animated_properties..AnimationValue as style..values..animated..Animate>::animate at src/liballoc/vec.rs:1713 #10: geckoservo::glue::compose_animation_segment at servo/ports/geckolib/glue.rs:578 #11: geckoservo::glue::Servo_AnimationCompose at servo/ports/geckolib/glue.rs:675 #12: mozilla::dom::KeyframeEffect::ComposeStyleRule(RawServoAnimationValueMap&, mozilla::AnimationProperty const&, mozilla::AnimationPropertySegment const&, mozilla::ComputedTiming const&) at dom/animation/KeyframeEffect.cpp:462 #13: mozilla::dom::KeyframeEffect::ComposeStyle(RawServoAnimationValueMap&, nsCSSPropertyIDSet const&) at dom/animation/KeyframeEffect.cpp:511 #14: mozilla::dom::Animation::ComposeStyle(RawServoAnimationValueMap&, nsCSSPropertyIDSet const&) at dom/animation/Animation.cpp:1090 #15: mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element const*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, RawServoAnimationValueMap*) at dom/animation/EffectCompositor.cpp:459 #16: Gecko_GetAnimationRule|layout/style/ServoBindings.cpp:560 #17: style::gecko::wrapper::get_animation_rule at servo/components/style/gecko/wrapper.rs:952 #18: style::matching::PrivateMatchMethods::replace_rules_internal at servo/components/style/matching.rs:182 #19: style::traversal::compute_style at servo/components/style/matching.rs:842 #20: _<style..gecko..traversal..RecalcStyleOnly<'recalc> as style..traversal..DomTraversal<style..gecko..wrapper..GeckoElement<'le>>>::process_preorder at servo/components/style/traversal.rs:434 #21: style::driver::traverse_dom at servo/components/style/driver.rs:111 ...

So this one is interesting. transform::get_normalized_vector_and_angle() is returning (0, 0, 0, 0). (rr) p x $27 = 3.40282347e+38 (rr) p y $28 = 2 (rr) p z $29 = 6 The reason is because the square length is not zero, but it's huge: (rr) p x * x + y * y + z * z $33 = 1.1579207543382391e+77 But then normalize returns (0., 0., 0.)... Birtles, this is arguably a degenerate case, do you know what's supposed to happen here?

Sorry for the delay. I'm afraid I don't know what's supposed to happen. This isn't my strong point and I can't find anything in the spec about this. Maybe Boris Chiou knows. If we can't do anything useful, then I wonder if returning the identity matrix (like we do when we can't normalize the vector) would be ok. It would at least be better than panicking.

This is only a debug assertion, so then probably not super-prioritary...

Hi Boris, is this something you want to look at? :-)

(In reply to Cameron McCormack (:heycam) (busy until July 12) from comment #4) > Hi Boris, is this something you want to look at? :-) Haha, ok.

If the vector has a squared length larger than float max, the rotate transform has an undefined behavior. I think it may be better to generate a similar rotate axis vector, instead of an identity matrix. The reasons are: 1. Our CSS parser truncates the infinity to float max, so the length of the potential-infinite-length vector could be something like f32::MAX. 2. I checked the same case in Google Chrome and Safari, they still generate an equivalent transform matrix, instead of an identity matrix. Therefore, we could scale the vector by f32::MAX (rust) or numeric_limits<float>::max() (c++), and then do the normalization, to get a valid unit vector.

Comment on attachment 8990159 [details] Bug 1467277 - Avoid getting zero normalized vector of rotate3d when setting a rotate matrix. https://reviewboard.mozilla.org/r/255170/#review262322 I think this approach looks reasonable. That's said, I wonder we can do this in BasePoint3D::Normalize instead?

Comment on attachment 8990158 [details] Bug 1467277 - Avoid getting zero normalized vector of rotate3d for animations. https://reviewboard.mozilla.org/r/255168/#review262324 I am wondering the same thing about this patch.

Comment on attachment 8990159 [details] Bug 1467277 - Avoid getting zero normalized vector of rotate3d when setting a rotate matrix. https://reviewboard.mozilla.org/r/255170/#review262322 Sounds good, I will try this way. Thanks.

Comment on attachment 8990158 [details] Bug 1467277 - Avoid getting zero normalized vector of rotate3d for animations. https://reviewboard.mozilla.org/r/255168/#review262682

Comment on attachment 8990159 [details] Bug 1467277 - Avoid getting zero normalized vector of rotate3d when setting a rotate matrix. https://reviewboard.mozilla.org/r/255170/#review262692

Comment on attachment 8990830 [details] Bug 1467277 - Bump euclid to 0.18 for style, style_traits, malloc_size_of, and tests. https://reviewboard.mozilla.org/r/255854/#review262764 ::: commit-message-3d20b:6 (Diff revision 1) > +Bug 1467277 - Bump euclid to 0.18 for style, style_traits, malloc_size_of, and tests. > + > +In order to drop old euclid version, we still need to bump euclid for > +plane-split and gfx/*. However, it needs more update and is not related to > +this bug, so let's do that in other place. Here, we bump euclid to > +0.18.1, and update style/values/generics/transform.rs for it. Could you file a bug for that? Thanks!

Comment on attachment 8990830 [details] Bug 1467277 - Bump euclid to 0.18 for style, style_traits, malloc_size_of, and tests. https://reviewboard.mozilla.org/r/255854/#review262764 > Could you file a bug for that? Thanks! Sure, Bug 1474646 will handle it. Thanks.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a57f16821e08 Bump euclid to 0.18 for style, style_traits, malloc_size_of, and tests. r=emilio https://hg.mozilla.org/integration/autoland/rev/e42f62ecacfe Avoid getting zero normalized vector of rotate3d when setting a rotate matrix. r=nical https://hg.mozilla.org/integration/autoland/rev/c17b3006ad32 Avoid getting zero normalized vector of rotate3d for animations. r=hiro

https://hg.mozilla.org/mozilla-central/rev/a57f16821e08 https://hg.mozilla.org/mozilla-central/rev/e42f62ecacfe https://hg.mozilla.org/mozilla-central/rev/c17b3006ad32

Is there a user impact which justifies backport consideration here?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25) > Is there a user impact which justifies backport consideration here? I think the answer is no. This should be safe. The only possible impact is: we bumped euclid crate for style, so we have two different versions of euclid in Gecko (one for style, another one for gfx). However, this should be ok.

That doesn't seem to answer the question about what the user impact of this bug is, which is what is relevant for backport decisions. Given that it's a fuzzer testcase, I'm guessing not worth backporting. Feel free to nominate if you feel strongly otherwise.